Pymc3 Plot & Diagnostics & Arviz Dependency by CloudChaoszero · Pull Request #4397 · pymc-devs/pymc

CloudChaoszero · 2021-01-01T11:00:47Z

Description

This PR starts the (Breakchange) removal of PyMC3 Plots and Diagnostics process referred in #4371, and relying on referred functionality from Arviz. See an example from the quote below

Plots and diagnostics are still accessible via PyMC3 but are actually implemented by ArviZ now. Although this was a good call when ArviZ was still nascent, now this causes confusion for new comers and overhead for devs, who have to mirror the functions in ArviZ, which is more work when ArviZ adds or changes functions (e.g pm.hpd vs. az.hdi).

The Implementation Process

Add Deprecation warning to one remaining (plot) function

Add a DeprecateWarning to function plot_posterior_predictive_glm

V4 3.11 implementation

Remove Diagnostics (pymc3/stats), incrementally
Remove Plots (pymc3/plots), incrementally
Test
Ensure Jupyter Notebook examples in pymc3-examples is updated as well

Notes

what are the (breaking) changes that this PR makes?
important background, or details about the implementation
are the changes—especially new features—covered by tests and docstrings?
consider adding/updating relevant example notebooks
right before it's ready to merge, mention the PR in the RELEASE-NOTES.md

review-notebook-app · 2021-01-01T11:00:54Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

CloudChaoszero · 2021-01-01T11:02:03Z

Note to self: Is v4 the base branch?

codecov · 2021-01-01T11:29:18Z

Codecov Report

Merging #4397 (1337d1b) into master (2824027) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master    #4397    +/-   ##
========================================
  Coverage   88.17%   88.17%            
========================================
  Files          88       87     -1     
  Lines       14563    14338   -225     
========================================
- Hits        12841    12643   -198     
+ Misses       1722     1695    -27

Impacted Files	Coverage Δ
pymc3/sampling.py	`89.72% <ø> (+0.10%)`	⬆️
pymc3/step_methods/mlda.py	`95.50% <ø> (-0.02%)`	⬇️
pymc3/plots/__init__.py	`100.00% <100.00%> (+46.93%)`	⬆️
pymc3/plots/posteriorplot.py	`96.00% <100.00%> (+0.34%)`	⬆️
pymc3/tests/sampler_fixtures.py	`96.80% <100.00%> (-0.20%)`	⬇️
pymc3/step_methods/hmc/integration.py	`89.79% <0.00%> (-2.05%)`	⬇️
pymc3/distributions/discrete.py	`94.25% <0.00%> (-1.75%)`	⬇️
pymc3/variational/approximations.py	`80.19% <0.00%> (-1.74%)`	⬇️
pymc3/step_methods/gibbs.py	`40.00% <0.00%> (-1.47%)`	⬇️
pymc3/tests/helpers.py	`60.34% <0.00%> (-1.33%)`	⬇️
... and 37 more

twiecki · 2021-01-02T21:08:58Z

Thanks @CloudChaoszero! I think we should make sure to also change the pymc-examples so that people can still run those fine. That should probably happen before we release with this change.

CloudChaoszero · 2021-01-03T11:42:32Z

Thanks @CloudChaoszero! I think we should make sure to also change the pymc-examples so that people can still run those fine. That should probably happen before we release with this change.

Sounds great. I'll proceed with that in the pymc-examples, and after update this branch.

OriolAbril

Everything looks good so far, I only have a minor suggestion about the api page. I'm looking forward to have the example notebooks updated.

docs/source/api.rst

pymc3/tests/test_examples.py

pymc3/plots/posteriorplot.py

michaelosthege

Hi @CloudChaoszero ,
I think this PR should move forward and get merged before the 3.11.0 release. We should keep just the plot_posterior_predictive_glm (and its tests) around because there is no replacement for it.

I think it's fair to remove the other plotting and stats functions, because we advertised ArviZ for a long time and even though they did not have deprecation warnings, their replacement is just a minor inconvenience for the users. 3.11 will bring much more severe breaking changes than this.

pymc3/plots/posteriorplot.py

docs/source/api.rst

CloudChaoszero · 2021-01-17T10:56:56Z

Thanks for the feedback y'all! 😄

Alright, so I implemented the following, based on reviews:

3.11

This PR is against master for 3.11 release.

Documentation

@OriolAbril
Added the plots section back into the documentation. Also, I changed the description to focus on arviz, but PyMC3 does offer plot_posterior_predictive_glm , seen below:

Keeping the plot_posterior_predictive_glm Function

@michaelosthege I went ahead and added a warning to plot_posterior_predictive_glm

    warnings.warn(
        "The `plot_posterior_predictive_glm` function will migrate to Arviz in a future release. "
        "\nKeep up to date with `ArviZ <https://arviz-devs.github.io/arviz/>`_ for future updates.",
        DeprecationWarning,
    )

@AlexAndorra Question: Are there any other Arviz diagnostics dependencies I should consider, outside of the plots topic?

AlexAndorra

Thanks @CloudChaoszero, starting to look good!
I added a few comments and suggestions below to make it ready to merge 😉

docs/source/about.rst

docs/source/api/plots.rst

docs/source/api/stats.rst

pymc3/plots/__init__.py

michaelosthege · 2021-01-17T12:01:23Z

@CloudChaoszero please try to bundle the commits. Every commit that is pushed causes the entire test pipeline to run, resulting in a lot of unnecessary compute.

When committing suggested changes from Github use the "add suggestion to batch" and when committing locally, try to push all commits at the same time - unless you want to trigger the CI for some specific reason.

Many currently running CI jobs can probably be cancelled..

CloudChaoszero · 2021-01-17T12:11:10Z

please try to bundle the commits. Every commit that is pushed causes the entire test pipeline to run, resulting in a lot of unnecessary compute.....

@michaelosthege Whoops, pardon! 😅 Thanks for the additional details.

@AlexAndorra Updated the PR. I am (slowly) also working on pymc-example PR to update docs, regarding this change.

MarcoGorelli

Nice - barring a couple of nitpicks, this looks good to me

MarcoGorelli · 2021-01-17T16:11:48Z

pymc3/plots/__init__.py

I don't think this needs to change (and if it does, I think the change needs doing for the whole codebase?)

Good point, let's not change that here then

MarcoGorelli · 2021-01-17T16:14:03Z

pymc3/plots/__init__.py

Suggested change

"exploratory analysis of Bayesian models." For more details, see https://arviz-devs.github.io/arviz/

"exploratory analysis of Bayesian models". For more details, see https://arviz-devs.github.io/arviz/ .

I would actually remove the " in exploratory analysis of Bayesian models.

MarcoGorelli · 2021-01-17T16:14:23Z

pymc3/plots/__init__.py

Suggested change

Only the `plot_posterior_predictive_glm` is kept in the PyMC code base for now, but it'll will move to ArviZ once the latter adds features for regression plot.

Only `plot_posterior_predictive_glm` is kept in the PyMC code base for now, but it will move to ArviZ once the latter adds features for regression plots.

AlexAndorra

Thanks for the quick update @CloudChaoszero ! Still a few typos to iron out

AlexAndorra · 2021-01-17T16:52:45Z

docs/source/api/plots.rst

Suggested change

The `plot_posterior_predictive_glm` function will removed in a future PyMC3 release.

The `plot_posterior_predictive_glm` function will be removed in a future PyMC3 release.

AlexAndorra · 2021-01-17T17:00:02Z

pymc3/plots/__init__.py

Good point, let's not change that here then

OriolAbril · 2021-01-17T20:59:19Z

pymc3/plots/__init__.py

I would actually remove the " in exploratory analysis of Bayesian models.

OriolAbril · 2021-01-17T21:14:11Z

docs/source/api/plots.rst

I would not use an sphinx warning here but inside its api docs.

"""Plot posterior predictive of a linear model. Parameters ---------- trace : arviz.InferenceData or MultiTrace Output of pm.sample() eval : array_like Array over which to evaluate lm lm : function, default: linear function Function mapping parameters at different points to their respective outputs. input: point, sample output: estimated value samples : int, default=30 How many posterior samples to draw. kwargs : mapping, optional Additional keyword arguments are passed to ``matplotlib.pyplot.plot()``. Warnings -------- The `plot_posterior_predictive_glm` function will removed in a future PyMC3 release. """

something like that.

Sounds great @OriolAbril I added it. However, the Warning did not show in my local version. 🤷

CloudChaoszero · 2021-01-18T06:28:28Z

Sounds great @AlexAndorra. I went ahead and also updated the plots docs to have the warning be withing the plot_posterior_predictive_glm , from recommendation from @ OriolAbril.

Example

OriolAbril

I take the warning in the api docs is working with the current docstring? The two images without the warning are the same and don't match the docstring text. Other than that everything looks great

pymc3/plots/posteriorplot.py

🔥 Remove arviz plots * Remove directly imported arviz plots used in pymc3 plots 🔥 Remove all plots from PyMC3 Plots module 🔥 Remove PyMC3 plots references in Docs 🎨 Mention Plotting & Diagnostics in API page and remove plots reference in __init__.py ⏪ Revert posterior_plot function, test, and docs 🎨 Add deprecation warning to posterior_plot function 🎨 Add context on plot import and import back into __init__.py ✏️ Add warning and details of posterior_plot added Update docs/source/api/plots.rst Co-authored-by: Alexandre ANDORRA <andorra.alexandre@gmail.com> Update docs/source/api/plots.rst Co-authored-by: Alexandre ANDORRA <andorra.alexandre@gmail.com> Update pymc3/plots/__init__.py Co-authored-by: Alexandre ANDORRA <andorra.alexandre@gmail.com> Update pymc3/plots/__init__.py Co-authored-by: Alexandre ANDORRA <andorra.alexandre@gmail.com> ✏️ Update docs to add stats.rst details ✏️ Minor docs notation for posterioplot function(s) 📝 Add breakline before docstring title

AlexAndorra

Thanks @CloudChaoszero ! Found one last typo. And does the warning display in the plot docs now?

pymc3/plots/posteriorplot.py

docs/source/api/plots.rst

CloudChaoszero · 2021-01-18T10:13:45Z

And does the warning display in the plot docs now?

Done! And @AlexAndorra I believe not.
However, that was in my local build. Unsure if the same will be displayed.

Quite odd that the Warnings section did not show. We do have the DeprecateWarning in the function though... 🤷 haha

AlexAndorra · 2021-01-18T15:14:17Z

Yeah but we definitely need this warning in the docs as well. And I don't see why it would not work for you locally but work when we recompile the new docs. What do you think @OriolAbril ?
The last thing we can do is add back the warning in the docs text (as well as keep it in the function docstrings).

MarcoGorelli · 2021-01-18T15:25:05Z

@CloudChaoszero did you do make clean before rebuilding? It appears fine for me

CloudChaoszero · 2021-01-19T02:28:03Z

@CloudChaoszero did you do make clean before rebuilding? It appears fine for me

@MarcoGorelli Hmm, maybe not. But yeah, that looks good to me! 👏 thanks for verifying.

CC: @AlexAndorra

AlexAndorra

Aaaah, perfect then, thanks for chiming in with some doc magic @MarcoGorelli 🧙‍♂️
This is good to merge now 🥳 Thanks @CloudChaoszero !

CloudChaoszero changed the base branch from v4 to master January 1, 2021 11:01

twiecki added this to the vNext (3.11.0) milestone Jan 2, 2021

CloudChaoszero force-pushed the pymc3-arviz-plot-diagnostics branch from 875f4b9 to 721af7c Compare January 3, 2021 11:40

OriolAbril reviewed Jan 3, 2021

View reviewed changes

docs/source/api.rst Outdated Show resolved Hide resolved

CloudChaoszero force-pushed the pymc3-arviz-plot-diagnostics branch from 721af7c to 2f788a6 Compare January 4, 2021 08:49

CloudChaoszero marked this pull request as ready for review January 4, 2021 17:39

OriolAbril reviewed Jan 4, 2021

View reviewed changes

pymc3/tests/test_examples.py Outdated Show resolved Hide resolved

MarcoGorelli reviewed Jan 10, 2021

View reviewed changes

pymc3/plots/posteriorplot.py Outdated Show resolved Hide resolved

michaelosthege requested changes Jan 15, 2021

View reviewed changes

pymc3/plots/posteriorplot.py Outdated Show resolved Hide resolved

docs/source/api.rst Outdated Show resolved Hide resolved

CloudChaoszero force-pushed the pymc3-arviz-plot-diagnostics branch from 2f788a6 to c588520 Compare January 17, 2021 10:40

CloudChaoszero requested review from AlexAndorra, OriolAbril and michaelosthege January 17, 2021 10:57

AlexAndorra linked an issue Jan 17, 2021 that may be closed by this pull request

Remove Plots & Diagnostics from PyMC Code Base #4371

Closed

AlexAndorra requested changes Jan 17, 2021

View reviewed changes

CloudChaoszero force-pushed the pymc3-arviz-plot-diagnostics branch from b5045c8 to c90a6f3 Compare January 17, 2021 12:09

CloudChaoszero requested review from AlexAndorra and MarcoGorelli January 17, 2021 13:34

MarcoGorelli approved these changes Jan 17, 2021

View reviewed changes

AlexAndorra requested changes Jan 17, 2021

View reviewed changes

OriolAbril reviewed Jan 17, 2021

View reviewed changes

CloudChaoszero requested review from AlexAndorra, MarcoGorelli and OriolAbril January 18, 2021 06:28

CloudChaoszero changed the title ~~[WIP] Pymc3 Plot & Diagnostics & Arviz Dependency~~ Pymc3 Plot & Diagnostics & Arviz Dependency Jan 18, 2021

OriolAbril approved these changes Jan 18, 2021

View reviewed changes

pymc3/plots/posteriorplot.py Outdated Show resolved Hide resolved

CloudChaoszero force-pushed the pymc3-arviz-plot-diagnostics branch from a4c8f5b to 8076612 Compare January 18, 2021 08:23

AlexAndorra requested changes Jan 18, 2021

View reviewed changes

pymc3/plots/posteriorplot.py Outdated Show resolved Hide resolved

docs/source/api/plots.rst Outdated Show resolved Hide resolved

✏️ small typo and remove summary in plots.rst

1337d1b

CloudChaoszero requested a review from AlexAndorra January 18, 2021 10:11

ricardoV94 mentioned this pull request Jan 19, 2021

pymc3.traceplot() method documentation error #3876

Closed

AlexAndorra approved these changes Jan 19, 2021

View reviewed changes

michaelosthege approved these changes Jan 19, 2021

View reviewed changes

AlexAndorra merged commit 37ca5ea into pymc-devs:master Jan 19, 2021

eigenfoo mentioned this pull request Jan 22, 2021

New commits to pymc3/sampling.py or pymc3/step_methods/hmc/ eigenfoo/littlemcmc#106

Closed

michaelosthege mentioned this pull request Jan 22, 2021

pymc3: unexpected minor version effect from 3.10.0 to 3.11.0 #4430

Closed

michaelosthege mentioned this pull request Mar 11, 2021

Bring back ArviZ wrappers with DeprecationWarnings #4528

Closed

	"exploratory analysis of Bayesian models." For more details, see https://arviz-devs.github.io/arviz/
	"exploratory analysis of Bayesian models". For more details, see https://arviz-devs.github.io/arviz/ .

	Only the `plot_posterior_predictive_glm` is kept in the PyMC code base for now, but it'll will move to ArviZ once the latter adds features for regression plot.
	Only `plot_posterior_predictive_glm` is kept in the PyMC code base for now, but it will move to ArviZ once the latter adds features for regression plots.

	The `plot_posterior_predictive_glm` function will removed in a future PyMC3 release.
	The `plot_posterior_predictive_glm` function will be removed in a future PyMC3 release.

Conversation

CloudChaoszero commented Jan 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

The Implementation Process

Add Deprecation warning to one remaining (plot) function

V4 3.11 implementation

Notes

Uh oh!

review-notebook-app bot commented Jan 1, 2021

Uh oh!

CloudChaoszero commented Jan 1, 2021

Uh oh!

codecov bot commented Jan 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

twiecki commented Jan 2, 2021

Uh oh!

CloudChaoszero commented Jan 3, 2021

Uh oh!

OriolAbril left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaelosthege left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CloudChaoszero commented Jan 17, 2021

3.11

Documentation

Keeping the plot_posterior_predictive_glm Function

Uh oh!

AlexAndorra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaelosthege commented Jan 17, 2021

Uh oh!

CloudChaoszero commented Jan 17, 2021

Uh oh!

MarcoGorelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoGorelli Jan 17, 2021 • edited by AlexAndorra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexAndorra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CloudChaoszero commented Jan 18, 2021

Uh oh!

CloudChaoszero commented Jan 1, 2021 •

edited

Loading

codecov bot commented Jan 1, 2021 •

edited

Loading

MarcoGorelli Jan 17, 2021 •

edited by AlexAndorra

Loading

CloudChaoszero commented Jan 18, 2021 •

edited

Loading